Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
| export function findConfigFileName(host: LinterHost) { | ||
| const children = new Set( | ||
| host.readDirectory(host.getCurrentDirectory()).map((file) => file.name), | ||
| ); |
There was a problem hiding this comment.
I think we need the host to be async-first, blocking second. @auvred, is there a reason why the host is synchronous-first beyond that typescript is synchronous? Can we rename readDirectory to like readDirectorySync for ts and make readDirectory the promised version?
Also is there a reason getCurrentDirectory is a function? It's always constant. I really couldn't care less, if it's a style choice that's fine by me, but I'm mildly curious. Wouldn't it be more efficient (on a nano-optimization level ofc) to make it property?
There was a problem hiding this comment.
I think we need the host to be async-first, blocking second. @auvred, is there a reason why the host is synchronous-first beyond that typescript is synchronous? Can we rename
readDirectoryto likereadDirectorySyncfor ts and makereadDirectorythe promised version?
Yeah, it's synchronous only because ts.sys is synchronous. Adding a pair of async methods sounds pretty reasonable to me. We probably won't gain much speed from using async versions, but it's nice to have IMO.
Also is there a reason
getCurrentDirectoryis a function? It's always constant. I really couldn't care less, if it's a style choice that's fine by me, but I'm mildly curious. Wouldn't it be more efficient (on a nano-optimization level ofc) to make it property?
Since our LinterHosts are interface-based and designed to be layered on top of one another, it's easier to have getCurrentDirectory as a function (some LinterHost implementation may choose to override its behavior dynamically)
|
We really need a better way to do stacked PRs, this is stacked on #2199 so I have to keep this in draft but that stops it from showing up the review queue when this is equally ready for review... 🤔 |
098c55e to
7bafdf6
Compare
7bafdf6 to
75d8f23
Compare
75d8f23 to
44e0308
Compare
| } | ||
|
|
||
| export interface WatchDirectoryOptions extends WatchOptions { | ||
| recursive: boolean; |
There was a problem hiding this comment.
WDYT about making recursive an optional? pollingInterval is optional, so it would be nice to be consistent. However, if you made this for the sake of explicitness, I'm okay with that!
There was a problem hiding this comment.
if you made this for the sake of explicitness, I'm okay with that!
Yeah, it was required before so I was sticking with that. I don't know enough about watching to know what it makes sense to default to (aka why is recursive: false even a thing?) so I'm not going to touch it.
If anyone else feels does strongly though, please let me know! I'm happy to change it.
44e0308 to
ee73947
Compare
0f05925 to
cd3cedf
Compare
auvred
left a comment
There was a problem hiding this comment.
Generally, I'm really positive about these changes!
| getFileTouchTimeSync() { | ||
| // TODO: uhh... this probably doesn't work amazingly | ||
| return Date.now(); | ||
| }, |
cd3cedf to
ac543b1
Compare
ac543b1 to
ac0f621
Compare
I think Auvred's right here? He's more right than me that's for sure.
Co-authored-by: auvred <61150013+auvred@users.noreply.github.com>
ac0f621 to
c641d80
Compare
Co-authored-by: auvred <61150013+auvred@users.noreply.github.com>
PR Checklist
status: accepting prsOverview
❤️🔥
I don't remember if this works, I whipped this up on a 6-hour car ride to Oklahoma yesterday.